[fix] File upload #850

cjpais · 2024-08-26T20:32:53Z

I have a simple HTML form which submits a file to a server action via multipart form upload. From the client side, the file looks proper, but when the server action receives it, the file is much larger? Is there some intermediate processing of this file or improper handling in the library?

client sent:

File {
  lastModified: 1724514738000
  name: "bara.jpeg"
  size: 2094593
  type: "image/jpeg"
}

server received:

File {
  size: 3715003,
  type: 'image/jpeg',
  name: 'bara.jpeg',
  lastModified: 1724698582443
}

I went through the code, and the bug largely was parsing the body as a string. Instead we have an array buffer so we can manipulate it more directly. It's possible it may still work as a string, but I believe the array buffer leaves more flexibility.

I've only tested directly in the application I've been building

vercel · 2024-08-26T20:32:57Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
waku	✅ Ready (Inspect)	Visit Preview	Aug 29, 2024 0:00am

codesandbox-ci · 2024-08-26T20:33:16Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

cjpais · 2024-08-26T22:05:14Z

i would guess the right thing to do is probably move to busboy, etc

dai-shi

Nice catch and this looks great. Thanks for working on it.

dai-shi · 2024-08-26T23:16:27Z

packages/waku/src/lib/utils/form.ts

@@ -1,8 +1,16 @@
 // TODO is this correct? better to use a library?
-export const parseFormData = (body: string, contentType: string) => {
+export const parseFormData = (body: ArrayBuffer, contentType: string) => {


Would you be able to create a test for this function?

will try, going on vacation soon so have a lot of other projects to wrap up

i've only minorly tested the code itself, but it appears to be working in the project i found the bug in

looking forward to it. take your time.

dai-shi · 2024-08-26T23:18:18Z

packages/waku/src/lib/renderers/rsc-renderer.ts

@@ -194,13 +194,14 @@ export async function renderRsc(

  let decodedBody: unknown | undefined = args.decodedBody;
  if (body) {
-    const bodyStr = await streamToString(body);
+    const bodyBuf = await streamToArrayBuffer(body);
+    const bodyStr = arrayBufferToString(bodyBuf);


We can delay calling this in the else-if block. We just need to check if the buffer has a length.

great, latest version should have this

Cool, thanks.

dai-shi · 2024-08-26T23:22:08Z

i would guess the right thing to do is probably move to busboy, etc

We used to use busboy as React lib uses it, but it's Node.js specific and Waku now supports non-Node.js envs.

cjpais · 2024-08-27T02:46:34Z

tests are a good starting point, but really need more. large files, etc.. the current code does not pass. the tests themselves need some review for accuracy as well

dai-shi

Yeah, those tests are good start. Thanks for working on it. It's hard to tell the accuracy, but it's noticeable.

2nd review 👇 . Maybe we need a test for arrayBufferToString.

packages/waku/src/lib/utils/stream.ts

dai-shi · 2024-08-27T07:25:53Z

packages/waku/src/lib/utils/stream.ts

@@ -31,6 +56,13 @@ export const streamToString = async (
  return outs.join('');
 };

+export function arrayBufferToString(buffer: ArrayBuffer): string {
+  const uint8Array = new Uint8Array(buffer);
+  return Array.from(uint8Array)


I'm not familiar but does it create a new array?

array.from does yes, I think we should try to avoid conversions if possible, or convert once and handle from there. this feels like a hack to me

dai-shi · 2024-08-27T07:37:18Z

packages/waku/src/lib/utils/stream.ts

+export function arrayBufferToString(buffer: ArrayBuffer): string {
+  const uint8Array = new Uint8Array(buffer);
+  return Array.from(uint8Array)
+    .map((byte) => String.fromCharCode(byte))


This doesn't seem to work for non-ASCII chars. Don't we need to use TextDecoder?

Do we have the same issue in parseFormData?

yep I believe so

cjpais · 2024-08-27T16:47:36Z

So I initially wrote this code as a hack to solve a specific issue for my project. I think after writing the tests and seeing some of the review comments, it might be nice to review this body of code in total and maybe write a better implementation to handle more cases (like binary, base64, quoted strings, etc).

Mentioned in discord but I see Hono is doing this. Not sure if applicable given support for non node.js based runtimes, I'm not super familiar with the API's down to that level. But since Hono supports different runtimes, I imagine there are either polyfills or something to get the parsing working.

export const bufferToFormData = (
  arrayBuffer: ArrayBuffer,
  contentType: string
): Promise<FormData> => {
  const response = new Response(arrayBuffer, {
    headers: {
      'Content-Type': contentType,
    },
  })
  return response.formData()
}

if I get a chance today I may try something as simple as this. (however I am using bun runtime so it probably has support for the node.js apis)

Edit: I couldn't help myself from trying. It looks to have worked, but still failing some tests (could be poorly written tests? as this is a canonical implementation??). If this is a valid way to go it is by far the simplest and probably most robust. I am going to submit the code I have shortly, as well as updated arrayBufferToString from Hono as well

packages/waku/src/lib/renderers/rsc-renderer.ts

cjpais · 2024-08-27T17:31:40Z

packages/waku/src/lib/utils/stream.ts

@@ -56,13 +56,6 @@ export const streamToString = async (
  return outs.join('');
 };

-export function arrayBufferToString(buffer: ArrayBuffer): string {


Hono provides this now

packages/waku/src/lib/utils/form.ts

packages/waku/src/lib/renderers/rsc-renderer.ts

dai-shi

Overall looks good. Thanks for working on it.

packages/waku/src/lib/utils/buffer.ts

packages/waku/src/lib/utils/stream.ts

…cjpais/fix-file-upload

dai-shi

This is so great! Thanks for your contribution!

packages/waku/src/lib/utils/buffer.ts

fix files in forms

fd1f48c

vercel bot deployed to Preview August 26, 2024 20:33 View deployment

fix bad decoding

6f819c0

vercel bot deployed to Preview August 26, 2024 21:08 View deployment

dai-shi reviewed Aug 26, 2024

View reviewed changes

move conversion into conditional

b6a773f

vercel bot deployed to Preview August 26, 2024 23:48 View deployment

wip tests

4868377

vercel bot deployed to Preview August 27, 2024 01:30 View deployment

more comprehensive tests, more work to be done

219f5f4

vercel bot deployed to Preview August 27, 2024 02:06 View deployment

dai-shi reviewed Aug 27, 2024

View reviewed changes

dai-shi mentioned this pull request Aug 27, 2024

v1 Roadmap #24

Open

use hono provided libs for decoding forms

ec5a4bb

cjpais commented Aug 27, 2024

View reviewed changes

packages/waku/src/lib/renderers/rsc-renderer.ts Show resolved Hide resolved

cjpais commented Aug 27, 2024

View reviewed changes

packages/waku/src/lib/utils/form.ts Show resolved Hide resolved

vercel bot deployed to Preview August 27, 2024 17:32 View deployment

dai-shi requested changes Aug 28, 2024

View reviewed changes

packages/waku/src/lib/renderers/rsc-renderer.ts Show resolved Hide resolved

cjpais added 3 commits August 27, 2024 19:52

add buffer.ts to replace hono/utils implementation

c8dd468

remove qp test, rewrite png based tests using request object

70bf54d

basic tests for buffer to string

e0816bd

vercel bot deployed to Preview August 28, 2024 03:18 View deployment

dai-shi reviewed Aug 28, 2024

View reviewed changes

packages/waku/src/lib/utils/buffer.ts Outdated Show resolved Hide resolved

packages/waku/src/lib/utils/stream.ts Show resolved Hide resolved

Merge branch 'main' into cjpais/fix-file-upload

96f17c3

vercel bot deployed to Preview August 28, 2024 03:55 View deployment

cjpais added 4 commits August 28, 2024 09:37

remove conditional check for buffer to string

41cd666

add fixme

fb44400

clean up test suite

0d5e4e6

Merge branch 'cjpais/fix-file-upload' of github.com:cjpais/waku into …

4262e29

…cjpais/fix-file-upload

vercel bot deployed to Preview August 28, 2024 16:47 View deployment

dai-shi approved these changes Aug 28, 2024

View reviewed changes

packages/waku/src/lib/utils/buffer.ts Outdated Show resolved Hide resolved

packages/waku/src/lib/utils/buffer.ts Outdated Show resolved Hide resolved

Apply suggestions from code review

4a9f9d8

vercel bot deployed to Preview August 29, 2024 00:00 View deployment

dai-shi merged commit 9c5a655 into dai-shi:main Aug 29, 2024
28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix] File upload #850

[fix] File upload #850

cjpais commented Aug 26, 2024

vercel bot commented Aug 26, 2024 •

edited

Loading

codesandbox-ci bot commented Aug 26, 2024 •

edited

Loading

cjpais commented Aug 26, 2024

dai-shi left a comment

dai-shi Aug 26, 2024

cjpais Aug 26, 2024 •

edited

Loading

dai-shi Aug 26, 2024

dai-shi Aug 26, 2024

cjpais Aug 26, 2024

dai-shi Aug 26, 2024

dai-shi commented Aug 26, 2024

cjpais commented Aug 27, 2024

dai-shi left a comment •

edited

Loading

dai-shi Aug 27, 2024

cjpais Aug 27, 2024

dai-shi Aug 27, 2024 •

edited

Loading

cjpais Aug 27, 2024 •

edited

Loading

cjpais commented Aug 27, 2024 •

edited

Loading

cjpais Aug 27, 2024

dai-shi left a comment

dai-shi left a comment

[fix] File upload #850

[fix] File upload #850

Conversation

cjpais commented Aug 26, 2024

vercel bot commented Aug 26, 2024 • edited Loading

codesandbox-ci bot commented Aug 26, 2024 • edited Loading

cjpais commented Aug 26, 2024

dai-shi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjpais Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dai-shi commented Aug 26, 2024

cjpais commented Aug 27, 2024

dai-shi left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dai-shi Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

cjpais Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

cjpais commented Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

dai-shi left a comment

Choose a reason for hiding this comment

dai-shi left a comment

Choose a reason for hiding this comment

vercel bot commented Aug 26, 2024 •

edited

Loading

codesandbox-ci bot commented Aug 26, 2024 •

edited

Loading

cjpais Aug 26, 2024 •

edited

Loading

dai-shi left a comment •

edited

Loading

dai-shi Aug 27, 2024 •

edited

Loading

cjpais Aug 27, 2024 •

edited

Loading

cjpais commented Aug 27, 2024 •

edited

Loading